fix(api-v1): return real task.status in CreateTaskResponse, not hardcoded "pending"#426
Conversation
… "pending" POST /v1/chat/tasks returned ``status="pending"`` in the response body even though ``begin_turn`` had already atomically claimed the row as RUNNING inside the same handler before the response was sent. An SDK client doing POST followed by an immediate GET on the same task would see two contradictory status values from back-to-back calls (``pending`` then ``running``). Read ``task.status.value`` after ``begin_turn`` returns instead -- ``begin_turn`` refreshes the in-memory ``task`` after committing the atomic claim, so the post-handler view reflects the row's real state. ``AppendMessageResponse`` already followed this contract; the create path now matches it. Schema docstrings for CreateTaskResponse and AppendMessageResponse updated to describe the post-claim ``running`` semantics; the ``test_create_task_happy_path`` assertion was checking for the old ``pending`` value and has been updated. No behavior change in the orchestrator or scheduler -- this is a response-payload correctness fix only. Twenty-eight v1 task tests pass; pre-commit (ruff / mypy / isort / codespell) green.
There was a problem hiding this comment.
Code Review
This pull request updates the task creation API to return the actual task status (typically 'running') instead of a hardcoded 'pending' value, ensuring consistency between the response and the database state. Documentation and tests have been updated to reflect this change. The reviewer suggested further improving consistency by replacing hardcoded status strings with the Enum value in other response models like AppendMessageResponse.
|
The title v1 is distracting, maybe call it api v1 or sth. |
…nse status read Follow-up to the CreateTaskResponse status fix in the previous commit. Two consistency cleanups within the same handler module: 1. ``create_chat_task`` function docstring still described the return value as ``status='pending'`` -- inconsistent with the schema docstring and the implementation that the previous commit already moved to ``task.status.value`` (i.e. 'running'). Updated the Returns section to match. 2. ``append_message_to_task`` returned ``status="running"`` hard- coded while ``create_chat_task`` reads ``task.status.value``. Unified both endpoints on the same pattern -- reading from the refreshed in-memory row is defensive (any future ``begin_turn`` status-machine change is picked up automatically) and removes the asymmetry where one endpoint reflected the DB and the other asserted a fixed string. No behavior change for the current contract -- the previous commit already returned 'running' from CREATE; this commit makes APPEND share the same expression form and fixes the docstring drift. Twenty-eight v1 task tests pass; pre-commit green.
|
Renamed the scope from |
…lure
``execute_task_background`` previously logged and broadcast the
exception text on failure but never wrote it to ``task.error_message``.
The row's status stayed RUNNING, and ``finish_turn``'s
RUNNING-fallback branch then unconditionally set
``error_message`` to a generic placeholder ("Task execution failed
without status update; see /steps."), forcing SDK and web clients
to fetch ``/steps`` to discover what actually went wrong.
The fix writes the real exception text to ``task.error_message`` and
flips ``status`` to ``FAILED`` in the exception handler, using a
fresh session because the original may be in a failed-transaction
state. ``finish_turn``'s FAILED branch then only fills in a
placeholder when ``error_message`` is empty, so the real message is
preserved through to the final row state.
Same family of fix as the ``status="pending"`` correction above:
make the persisted task row reflect the real outcome rather than a
generic placeholder. Affects both SDK consumers (GET /v1/chat/tasks/
{id}) and web/WebSocket consumers reading the same row.
Twenty-eight v1 task tests pass; pre-commit (ruff / mypy / isort /
codespell) green.
rogercloud
left a comment
There was a problem hiding this comment.
I found one issue in the failure handling path.
…site execute_task_background's outer except spans post-terminal steps (assistant-message persistence and the completion/paused broadcasts) that run after the task status was already committed COMPLETED. The recently added FAILED-persistence wrote the row unconditionally, so a failure in one of those best-effort steps -- e.g. the completion broadcast losing its websocket -- rewrote an already-completed task as FAILED and stored the broadcast error in error_message. Branch the handler on the task's current status instead. Only a task still RUNNING is a genuine execution failure: record the real exception text, flip to FAILED, and emit task_error. A task already in a terminal state tripped here in a best-effort post-completion step, so observe it without touching the row or emitting a contradictory task_error; finish_turn still reconciles the terminal fields afterward.
Single content conflict in websocket.py's execute_task_background failure handler. Adopt upstream's _terminal_task_error_payload (which persists FAILED + the real error_message and builds the notification payload) as the genuine-failure path, but gate it on the task still being RUNNING. The outer except also spans post-completion best-effort steps (assistant-message persistence, completion/paused broadcasts); a failure there must not rewrite an already-terminal task as FAILED or emit a contradictory task_error -- it is logged and the row is left for finish_turn to reconcile.
rogercloud
left a comment
There was a problem hiding this comment.
Follow-up on the narrowed failure boundary.
The success path committed COMPLETED/FAILED before persisting the assistant message, which is a separate durable write. If that write failed, the row was left COMPLETED with no message and no error_message -- the status-gated failure handler treated it as a best-effort post-completion step and left it untouched. Leave the terminal status pending and let persist_assistant_message's commit land it atomically with the message. A failure in that durable write now leaves the status RUNNING, so the outer handler surfaces a real task failure instead of a contradictory empty COMPLETED row. Only notification broadcasts remain best-effort.
The previous change rode the terminal-status commit on persist_assistant_message's internal commit. But that helper early-returns without committing when the assistant content is empty (a valid empty-reply turn), which left the status pending -> RUNNING -> finish_turn flipping a successful empty turn to FAILED. Add an explicit commit after persistence. It lands the terminal status whether or not a message row was written, while still surfacing a real failure when persistence raises (control never reaches the explicit commit, so the status stays uncommitted and the outer except fails it).
rogercloud
left a comment
There was a problem hiding this comment.
Reviewed the current head c0aa960. No findings; targeted local tests and GitHub checks are green.
Summary
Two related response-correctness fixes for the v1 SDK API surface — both make the task row reflect the real outcome instead of a hardcoded placeholder.
Fix 1 —
statusfield onPOST /v1/chat/tasksPOST /v1/chat/tasksreturnedstatus="pending"in the response body even thoughbegin_turnhad already atomically claimed the row asRUNNINGinside the same handler before the response was sent. An SDK client doingPOSTfollowed by an immediateGETon the same task would see two contradictory status values from back-to-back calls (pendingthenrunning).Reads
task.status.valueafterbegin_turnreturns.begin_turnrefreshes the in-memorytaskafter committing the atomic claim, so the post-handler view reflects the row's real state.AppendMessageResponsealready followed this contract; the create path now matches it, and the create-side response-string was unified to read from the refreshed row so both endpoints use the same expression form.Fix 2 —
task.error_messagepopulated with real exception textexecute_task_backgroundpreviously logged and broadcast the exception text on failure but never wrote it totask.error_message. The row's status stayedRUNNING, andfinish_turn's RUNNING-fallback branch then unconditionally seterror_messageto a generic placeholder ("Task execution failed without status update; see /steps."), forcing SDK and web clients to fetch/stepsto discover what actually went wrong.The fix writes the real exception text to
task.error_messageand flipsstatustoFAILEDin the exception handler, using a fresh session because the original may be in a failed-transaction state.finish_turn's FAILED branch then only fills in a placeholder whenerror_messageis empty, so the real message is preserved through to the final row state.Changes
src/xagent/web/api/v1/tasks.py:POST /v1/chat/tasksreadstask.status.valueinstead of hardcoded"pending"; brief comment mirrors the equivalent block at theAppendMessageResponsereturn site.POST /v1/chat/tasks/{id}/messagesswitched to the sametask.status.valuepattern so the two endpoints are symmetric (was hardcoded"running").create_chat_taskReturns:docstring updated to describe the post-claimstatus='running'contract.src/xagent/web/schemas/v1.py:CreateTaskResponseandAppendMessageResponsefield docstrings updated — both previously claimed"always 'pending'", both now describe the post-claimrunningsemantics.src/xagent/web/api/websocket.py:execute_task_backgroundexception handler now writesstr(e)[:4000]totask.error_messageand flipsstatustoFAILEDin a fresh session before broadcasting the error event.tests/web/api/v1/test_tasks.py:test_create_task_happy_pathwas asserting the old"pending"value and has been updated to assert"running", with a comment explaining the post-claim semantics.Why not a regression risk
statusanderror_messagecolumns; both are now accurate where they were previously a placeholder.AppendMessageResponsealready returned"running"for the same reason, so SDK clients tolerating that one will tolerate the create-side identically.Test plan
pytest tests/web/api/v1/ tests/web/services/ tests/web/api/test_agent_api_keys.py— 92 passedpre-commit run --files <touched files>— ruff / mypy / isort / codespell greenfinish_turn's FAILED branch preserves the populatederror_message(only writes a placeholder when empty).